-
Notifications
You must be signed in to change notification settings - Fork 394
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allowed title as props for <details>
#3388
Conversation
question if this approach breaks some titles and we have to maintain two options, why do we consider it superior and why do ? let's keep the previous one? also, it's not at all a high priority task to my mind, folks. Unless something is broken now. |
markdown wouldn't be supported when passed as props but I think it would be okay without it too as it's used in just 2 titles to highlight
I think this one needs to be addressed and which we didn't address on pervious pr. |
Still, I don't see any clear motivation for this tbh. And still it breaks things in some cases. So, my take - let's postpone / keep it as-is for now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a bad idea, but I think it would be better to separate the bugfixes like the object object
thing and the prop-title feature into different PRs, the former is an easy fix we'll want asap while the latter needs some more discussion.
<details> | ||
<details title="Click and expand to set up the example"> | ||
|
||
### Click and expand to set up the project |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<details> | ||
|
||
### What is a "local remote" ? | ||
<details title=' What is a "local remote" ?'> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it looks alright since the result is trimmed, we should trim out the excess spaces here.
<details> | ||
|
||
### Click and expand to set up the project | ||
<details title="Click and expand to set up the project> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this PR update all existing <details>
blocks? Thanks
This comment was marked as resolved.
This comment was marked as resolved.
Reminder:
can we skip addressing the conflict? please please let's be mindful about the bigger priorities |
### Click and expand to set up example | ||
<details title="Click and expand to set up example"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK just noticed we're postponing this. Just a note that per #3391 these titles should no longer start in "Click for" nor end in "example" (same throughout docs codebase). Thanks
Given this is not much of a priority + all the conflicts, should we close it or turn it into an issue? Thanks |
From #3383
Used title as props for
<details>
tag.Initially, I only used
title
as props but there are 2-3 particular cases that usecode
highlighting in the title.So, I also added back the first child's text as a fallback if no title is passed as props so that we still can use markdown as well.
Also, previously in this particular case, the code section seems to be parsed as
object object
as wellresulting: https://dvc.org/doc/user-guide/external-dependencies#expand-to-see-resulting-object-object-file
This issue should also be fixed with the pr.
But, if markdown syntax is not so important and the title would be enough then the code implementation of the Details component will be very simple and preferable.